Add multiple scopes support to string-format rule #1009
Conversation
| if err != nil { | ||
| w.parseError("unable to parse argument number in rule scope", ruleNum, 0) | ||
| // Parse rule scopes | ||
| rawScopes := strings.Split(rule[0], ",") |
There was a problem hiding this comment.
I'm a bit worried about the fact people may use , , or ,, or , as delimiter.
So maybe, you could use strings.TrimSpace on the rawScope in the for loop
Also, xhecking that the rawScope is not empty could avoid surprises and bugs
I'm pretty sure someone would face a
foo,,bar, or foo , , bar
No matter the implementation choice to either clean them, or forbid them. It should be covered by a test
|
I like the proposal and the way you implemented it, I found a few things that might be improved maybe |
|
Add behavior from comment |
| regexp *regexp.Regexp | ||
| negated bool | ||
| errorMessage string | ||
| } | ||
|
|
||
| type stringFormatSubruleScopes []*stringFormatSubruleScope |
There was a problem hiding this comment.
out of curiosity: why is that a slice of pointers? (not a slice of values)
There was a problem hiding this comment.
I saw one video on youtube where this two approaches were compared and slice of pointers use less memory little bit)
|
@chavacava Hi, could you review my PR, please) |
RULES_DESCRIPTIONS.md
Outdated
| @@ -775,6 +775,7 @@ Example: | |||
| ["core.WriteError[1].Message", "/^([^A-Z]|$)/", "must not start with a capital letter"], | |||
| ["fmt.Errorf[0]", "/(^|[^\\.!?])$/", "must not end in punctuation"], | |||
| ["panic", "/^[^\\n]*$/", "must not contain line breaks"], | |||
| ["fmt.Errorf[0],core.WriteError[1].Message", "/^.*%w.*$/", "must not contains '%w'"], | |||
There was a problem hiding this comment.
| ["fmt.Errorf[0],core.WriteError[1].Message", "/^.*%w.*$/", "must not contains '%w'"], | |
| ["fmt.Errorf[0],core.WriteError[1].Message", "/^.*%w.*$/", "must not contain '%w'"], |
Using this conf the rule (wrongly) matches things like
return fmt.Errorf("not implemented: AlterableDoltTable.DropPrimaryKey()"
|
Thanks for the PR! |
|
I made mistake in rule example. I fixed it, you can check again. Regexp rule should be inversed to lint only strings which contains '%w' |
Hi, I added new feature.
I describe my motivation in issue.
I added some tests and refactor old one little bit, becouse change error message.
My code almost follow the coding style of the rest of the repository.
Build passed.
Closes #1008